fix(pr-pipeline): hold reuse PRs on real conditions and leak-scan the reused branch by kyle-sexton · Pull Request #10624 · melodic-software/claude-code-plugins

kyle-sexton · 2026-10-28T07:29:38Z

No related issue: an owner-directed change to the work-resolve-issue reuse path. It supersedes the fixed reuse hold from #7831.

Summary

Until now, every reuse PR that work-resolve-issue opened from a branch an earlier run left with commits and no report carried Hold: a human merges this, whatever the facts. reusePlan opened the PR whatever the runner checks returned, and main() never read merge: human on a reuse.

This PR replaces that fixed hold. The reused branch's commits get a leak scan at reuse, and the PR holds only on the real conditions, through one mustHold check that decide() and reusePlan() both call.

Superseded: #7831 kept the automatic hold "until #7815 is proven on live runs". The leak scan now closes the gap that hold covered, and every real hold condition is kept.

Fix

report-outcome.mjs exports mustHold. The PR holds when:
- a trusted labeler set merge: human, now read on a reuse too and failing closed on an unreadable event list;
- the issue is labeled work-class: untrusted-provenance;
- the denied paths can't be read, or the compare failed;
- the listing stops at the 300-file cap while some path is denied;
- a changed path is denied;
- the runner checks are missing or any check did not pass;
- on a reuse, the leak scan is missing or not clean.
readChecks reads an optional scan word: clean, found or failed. An unknown word reads as failed.
A new step in work-resolve-issue.yml, run-checks > "Scan the reused branch for credentials", runs only on a reuse and before any check. It calls the new scan-reuse.mjs from the base checkout, which scans the diff from the branch's merge base with the base tip and every file that diff makes, by name and bytes. The checks step writes the word into the record, or failed when the scan covered another tip or set no word. The job keeps its existing permissions: no new token scope.
The read runner's scan logic moved from the inline leak-scan script in pr-run-activity-read.yml into lib/publish-gate.mjs as scanLeak, deeperLeak and changedBytes. That covers the per-line classification, the UTF-16LE pass, the split-line pass and the scan of the files a patch makes. The scan job loads these the same way as before (API, default-branch tip, imports only node: builtins), so both scans run one copy and there is no second copy of the patterns. The review and visualization copies of the gate were regenerated with scripts/sync-shared-copies.sh.
pr-run-activity.md now describes the reuse hold and the reuse scan. GAPS X5 stays only on the resumed-run half of that bullet, which is still open.

Verification

callers.test.mjs: 195/195 pass both before and after the move, with the same pass set. Every scanWith behavior test passes with its text unchanged, including the warn-only label, UTF-16LE, split-line and binary-hunk tests. Two structural tests now point at the module: the minimal-gate test and the test that sliced deeper out of the inline script.
report-outcome.test.mjs: 283 pass. The 21 new tests all fail against origin/main code. They cover each hold condition, each paired with a clean reuse that does not hold; decide()/reusePlan() agreement; readChecks; main-level tests for scan, denied path, failed compare and merge: human; the run-checks record; the reuse-scan step end to end; and scan-reuse.mjs failure handling.
Other suites:
- lib/publish-gate.test.mjs plus every work-resolve-issue suite: 442 pass;
- explain-change.test.mjs and write-verdict.test.mjs: 156 pass;
- visualization present.test.mjs: 52 pass;
- merge-holds.test.mjs: 41 pass;
- test_merge_holds_sync.py: OK.
Checks: actionlint is clean on both workflows. sync-shared-copies.sh --check and --check-bump origin/main pass, as do validate-plugins.sh --only "review visualization" and check-changelog-fragments.sh.

Left out:

Only the merge-base diff is scanned, not each commit on the way to it.
A plain password or secret assignment holds a reuse PR, though the read runner only warns on it. callers.test.mjs already trips that warning on main, so a reused branch touching it holds.

… reused branch A reuse PR from work-resolve-issue carried "Hold: a human merges this" on every PR, and reusePlan opened it whatever the runner checks said. Both paths now ask one mustHold check in report-outcome.mjs: a trusted merge: human (now read on a reuse too, failing closed on unreadable events), untrusted-provenance, unreadable denied paths or changed-file list, a capped listing while a path is denied, a denied path, missing or failed runner checks, and on a reuse a leak scan that is missing or not clean. run-checks scans a reused branch before its checks run, with the read runner's own scan on the merge-base diff and every file it makes, and records the word in the checks record. The scan's passes move from the read runner's inline script into lib/publish-gate.mjs (scanLeak, deeperLeak, changedBytes), which the scan job already loads at the default branch tip, so both scans run one copy. The read runner's behavior tests pass unchanged. Supersedes the fixed hold from #7831. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… GAPS X5 on resume scan-reuse.mjs prints the read runner's "rotate this credential" line beside a credential-format label, and the resumed-run half of the doc bullet keeps its GAPS X5 tag, since that gap stays open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ntry points Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chatgpt-codex-connector · 2026-10-28T07:33:33Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-28T07:35:26.896370Z	`34ce1a2`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

github-actions · 2026-10-28T07:35:34Z

claude-review reviewed 34ce1a2848e79516d6e7a5ca4f0f16304117c098: 1 finding(s), 0 as inline comments.

I read the full diff and the touched code in report-outcome.mjs and work-resolve-issue.yml. I ran no tests or tools myself. The test results in the PR description are the author's claims, and I did not confirm them.

The leak-scan move into lib/publish-gate.mjs matches the old inline code line for line. I found no behavior change there. The new reuse-scan step fails closed. A missing merge base, a scan of another tip, an unknown word, or a thrown error all produce failed, which holds the PR. decide() and reusePlan() now share one mustHold.

One finding is inline: a reuse with zero runner checks is not held (report-outcome.mjs:126).

Not a finding, but check before merge: pr-run-activity-read.yml now destructures scanLeak and changedBytes from the gate it reads at the default-branch tip. If a run uses this workflow from the PR head while the default-branch gate still lacks those exports, the scan job fails. I could not tell from the diff whether that can happen. The old inline script needed only findSecret and SECRET_PATTERNS. Merging the gate and the workflow in one commit avoids the gap.

Spend: this change adds no paid resources. The extra run-checks step uses runner minutes already in the plan.

Manifest items: none hit, since no plugin.json or marketplace.json changed. The patch-level changelog fragments for the pr-pipeline, review and visualization plugins fit the change.

critical-findings: 1

Findings not posted inline (their lines are not in the diff):

.github/scripts/work-resolve-issue/report-outcome.mjs:126 (RIGHT): checkFailures(checks) is empty when checks.results is [], so a reuse whose checks record holds zero results passes this line. If the scan is clean, the PR then opens with no hold line and no check ever ran. The PR description says the hold fires when "the runner checks are missing or any check did not pass". The resumed path already treats zero results as not passing: checksPassed at line 700 requires results.length. readChecks accepts an empty results, and the readChecks test at the new record(...) helper builds one. To reproduce, call reusePlan({...CLEAN_REUSE, checks: {tip: TIP, results: [], scan: "clean"}}). The body has no HOLD_LINE. Hold on !checks.results.length here, or say in a comment that a reuse with no configured checks is intended to open unheld.

github-actions · 2026-10-28T07:37:07Z

claude-security-review reviewed 34ce1a2848e79516d6e7a5ca4f0f16304117c098: 2 finding(s), 0 as inline comments.

Security review: PR #10624

The change removes the unconditional Hold: a human merges this line from reuse PRs. It adds a leak scan of the reused branch. Both PR paths now call one mustHold. It also moves the read runner's leak-scan logic into lib/publish-gate.mjs.

I treated this as a weakened merge hold. report-outcome.mjs is a trusted path, and HOLD_LINE is what merge-holds.mjs (LANE_HOLD) and enable-auto-merge.mjs read to keep a PR from merging. I checked the new hold conditions, the scan step, the scan script and the moved gate code.

I found two defense-in-depth gaps in the reuse path and no CRITICAL finding. Neither is CRITICAL. Both need repository write access to a lane branch, and the denied-path hold, the runner checks and CI still run after them. This PR does not weaken merge-holds.mjs, enable-auto-merge.mjs or pr-merge.yml, and I checked them only as the consumers of the hold line.

critical-findings: 0

Findings

IMPORTANT: the reuse PR is not pinned to the tip the runner scanned and checked (report-outcome.mjs:826). See the inline comment.
IMPORTANT: the reuse path accepts a branch whose earlier commits are not all the lanes App's (report-outcome.mjs:709). See the inline comment.

What I checked and found sound

Scan step (work-resolve-issue.yml, "Scan the reused branch for credentials"):
- It runs scan-reuse.mjs and publish-gate.mjs from the .base checkout, not from the lane branch.
- The word that reaches GITHUB_OUTPUT is only clean, found or failed. The run-checks step re-validates it with ^(clean|found|failed)$ and ties it to the tip it scanned, otherwise failed.
- readChecks maps an unknown word to failed. mustHold holds on a missing or non-clean scan.
Git plumbing in scan-reuse.mjs: it blanks the config, disables hooks and fsmonitor, and passes --no-ext-diff and --no-textconv. A branch's .gitattributes therefore cannot run a driver. A scan error becomes failed, which holds the PR.
Moved scan logic:
- pr-run-activity-read.yml still loads the gate from the default-branch tip via the API and still rejects non-node: imports.
- The judge call sites match the new signature (judge(kind, bytes, diff)).
- The bytes passed to scanLeak are Buffers, so the latin1 decode is unchanged.
- The review and visualization copies of publish-gate.mjs match the root copy.
Hold conditions: humanMerge is now read on a reuse and fails closed when the events can't be read. A failed compare yields changedFiles = null, which holds. The denied-path and capped-listing conditions are unchanged from decide().

Not counted

Spend: the reuse scan adds a node step and some git reads to a run that already exists. It uses no extra model or API credit.

manifest fields: no plugin.json or marketplace.json change in this diff.

Note: I did not read docs/conventions/instruction-exception-register/README.md. The diff deletes no instruction text, only the fixed reuse hold in code. The doc edit in pr-run-activity.md only restates the new behavior.

Findings not posted inline (their lines are not in the diff):

.github/scripts/work-resolve-issue/report-outcome.mjs:826 (RIGHT): severity: IMPORTANT

reusePlan returns no tip, unlike decide(), which returns tip: checks.tip for an open-pr (around line 935). The post-open guard at line 1799 (if (plan.tip && pull.head?.sha !== plan.tip)) therefore never runs for a reuse PR.

Now that a clean reuse PR carries no hold line, the only vetting of its head is the leak scan and checks at the tip run-checks saw. main() compares the branch ref to checks.tip once, at lines 1642-1646. After that it makes several more API calls: compare() by branch name, the events pagination and GET /pulls. Then it opens the PR from the branch name.

If someone with push access moves the branch in that window, the PR opens with no hold. Its head is an unscanned tip, and changedFiles was computed from that moved branch. The denied-path hold, merge-holds, CI and the required checks on the PR still run, so I rate this IMPORTANT and not CRITICAL.

Suggested fix: pin the plan to the checked tip. Return tip: checks?.tip from reusePlan when checks is present, and call compare(checks.tip) the way the resumed path does at line 1663.

When hold is true the PR is held anyway, but the pin is harmless there.

.github/scripts/work-resolve-issue/report-outcome.mjs:709 (RIGHT): severity: IMPORTANT

This removes the unconditional hold on a reused branch. Whether the branch qualifies is decided by findPrior, where usable() requires only that the branch tip be a verified lanes-App commit (laneTip, line 1177-1179). The comment above allLaneCommits (line 1101) says why that is not enough: a carried-over branch "carries every commit ahead of the default branch into the next PR, so each of them, not only the tip, must be the lanes App's". The resume path enforces that (allLane, line 1183, and lines 1664-1667). The reuse path does not, and mustHold does not check it either.

Until now the fixed hold covered this gap, since a person reviewed every reuse PR. A reuse PR with a clean scan, passing checks and no denied path now opens with no hold. The lanes App's PRs are not held for human review, so any non-App commit on the branch would ride in as the App's.

I could not construct a path that gets a non-App commit under an App tip. A human commit pushed on top would itself become the tip and fail laneTip, and the App only commits on its own run branch or a resume that passes allLane. For that reason I rate this IMPORTANT and not CRITICAL.

Suggested fix: keep the invariant in one place by holding a reuse when allLaneCommits(commits, bot) is false. reusePlan already receives the compare result's commits through compare(). Alternatively, make findPrior require allLane(branch) for a reuse as it does for a resume.

github-actions · 2026-10-28T14:27:56Z

Post-merge mutation testing

Range: 5873c6b..abbc1c0
Merged pull request: #10624
Outcome: reported

Killed 2, survived 4, timeout 0, no coverage 0, invalid 0, not run 2.
Covered-code score 33%; mutation score 33%.

File	Killed	Survived	Not run
`.github/scripts/work-resolve-issue/report-outcome.mjs`	0	0	2
`.github/scripts/work-resolve-issue/scan-reuse.mjs`	1	1	0
`lib/publish-gate.mjs`	1	1	0
`plugins/review/lib/publish-gate.mjs`	0	1	0
`plugins/visualization/lib/publish-gate.mjs`	0	1	0

Surviving mutants

Location	Operator	Disposition	Cause
`.github/scripts/work-resolve-issue/scan-reuse.mjs:60`	ROR	unclassified	none
`lib/publish-gate.mjs:156`	ROR	unclassified	none
`plugins/review/lib/publish-gate.mjs:213`	SBR	unclassified	none
`plugins/visualization/lib/publish-gate.mjs:176`	ROR	unclassified	none

The run's summary has the full report.

kyle-sexton and others added 3 commits October 28, 2026 03:15

chore(pr-pipeline): mark scan-reuse.mjs executable like its sibling e…

34ce1a2

…ntry points Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

kyle-sexton marked this pull request as ready for review October 28, 2026 07:33

kyle-sexton added this pull request to the merge queue Oct 28, 2026

Merged via the queue into main with commit abbc1c0 Oct 28, 2026
77 checks passed

kyle-sexton deleted the fix/recovery-hold-paths branch October 28, 2026 13:53

kyle-sexton mentioned this pull request Oct 28, 2026

fix(work-resolve-issue): stop holding a reuse PR only because its runner checks did not report #10701

Merged

melodic-automation-lanes Bot mentioned this pull request Oct 29, 2026

fix(pr-pipeline): reuse or delete a stopped lane run's branch when the issue re-runs #7948

Closed

kyle-sexton mentioned this pull request Oct 30, 2026

fix(pr-pipeline): delete a stopped lane run's branch when the issue re-runs #11224

Merged

Navigation Menu

Repository navigation

fix(pr-pipeline): hold reuse PRs on real conditions and leak-scan the reused branch - #10624

fix(pr-pipeline): hold reuse PRs on real conditions and leak-scan the reused branch#10624
kyle-sexton merged 3 commits into
mainfrom
fix/recovery-hold-paths

kyle-sexton commented Oct 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 28, 2026

Uh oh!

github-actions Bot commented Oct 28, 2026

Uh oh!

Uh oh!

github-actions Bot commented Oct 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kyle-sexton commented Oct 28, 2026

Summary

Fix

Verification

Related

Uh oh!

chatgpt-codex-connector Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

github-actions Bot commented Oct 28, 2026

Uh oh!

github-actions Bot commented Oct 28, 2026

Security review: PR #10624

Findings

What I checked and found sound

Not counted

Uh oh!

Uh oh!

github-actions Bot commented Oct 28, 2026

Post-merge mutation testing

Surviving mutants

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chatgpt-codex-connector Bot commented Oct 28, 2026 •

edited

Loading